Could we introduce k_cycle_get_64() instead of this internal function?

Yes.

wouldn't it be the same if it just returns (RTC->COUNTER - (u32_t)last_cycles) & COUNTER_MASK)

Yes.

as this function would be used for timestamping in logging, i would like to see it optimized, without spin lock. My idea was following:

• we need to take into account that rtc wraps and we may just hit the case when it wrapped but rtc interrupt wasn't yet handled.
• once in the counter span there is a synchronization moment when last_reference32 is stored. It means that when z_timer_cycle_get_32() is called it will return value which is always bigger than ref_timestamp32 unless: RTC wrap just occurred.
• upperstamp32 - upper 8 bits of the timestamp, incremented (by 2^24) on each wrap. when upperstamp32 wraps ref_timestamp32 is also updated.
  With those in mind getting 32 bit timestamp can be following:

u32_t ret = RTC->COUNTER + `upperstamp32`;
if (ret < last_reference32) {
    // that's the special, very rare case wrapping of 24 bit or wrapping which is not yet handled by interrupt
   ret += COUNTER_SPAN;
}
return ret;

Maybe. My testing didn't show a huge impact from using the spinlock. I'd have to think hard about your algorithm to be sure it actually works: there is no last_reference32 and what it means affects correctness. The lock and re-using sysclock_get_32() is much easier to understand.

i think this could also be optimized. we read the timestamp, if it's smaller than last one it means that wrapping is not yet handled (assuming that reference timestamping happens at least once during each subcycle)

as i'm not the fan of ifdefs i would prefer to see if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL)) return;.

Meh. It's a really large sequence of code to stick into a conditional statement. If this solution has legs I'll consider it along with other perspectives.

Yeah, that works, though I'm not a fan of multiple exits.

But this is queued for next revision, along with replacing #ifdef in z_clock_elapsed() where it makes more sense.

i would rather assert here. Do we expect that to be null?

No, but user code gets here and if it isn't checked we'd get a hard fault. I'm not a fan of assert since it fails at runtime and Zephyr doesn't have a robust infrastructure for handling failure safely. This way nothing gets corrupted and the application has a chance to detect what went wrong.

if alarm is ready (configured too late) alarm_update_deadline_ will end up setting CC[0] to 4 ticks from now and ready alarm will expire up to 128us from now.

Huh? No, the ready list isn't what's used to calculate the deadline. Once it's on the ready list it will be processed at the next opportunity, without affecting the compare register.

i referred to the following scenario:
z_impl_k_alarm_schedule is called late. alarm is linked to ready_list and state is set to READY. update_deadline is set to true and z_alarm_update_deadline is called. in alarm_update_deadline_ rc=0 so compare==now but RTC->EVENTS_COMPARE[0] is not set. We end up with compare = now + COUNTER_MIN_DELTA being set in CC register. RTC interrupt will happen 4 ticks from now and alarm will be triggered from there.

OK, I understand the case now. Thanks for explaining it.

Yes, the alarm will fire late. This is no different from a sequence where the alarm was scheduled in the future and has reached its deadline by the time the counter is set; or when the delay occurred because something blocked the timer ISR for several ticks.

These are edge cases, and I believe they're handled in a reasonable way with reasonable complexity.

I would love it if there were some way to force a compare event to occur immediately, but AFAICT the ARM/Nordic peripherals don't allow that, so the only solution is to set the compare event to the shortest value we can be sure will fire without lapping. (MSP430 does allow triggering interrupts manually, which made things like this really easy to deal with when I was working in that world.)

what if instead of calling alarm_update_deadline_ in case of readiness interrupt would be triggered (NVIC_SetPending). Or that can be part of alarm_update_deadline_

That could trigger the ISR, but is there a way to force EVENTS_COMPARE[0] to be set?

This approach would work if we only used the ISR to process CC[0]. I would rather not limit it that way, as I can see use cases for low-latency dedicated callbacks triggered by system time that would use the other channels and bypass the alarm infrastructure, as a Nordic-specific extension (e.g. this might eliminate the need for the Bluetooth stack to maintain its own RTC).

But it is a potential solution if there's a strong consensus that the delay in this case is unacceptable. I personally don't consider it a serious problem that's worth excluding low-latency time-triggered callbacks.

algorithm does not take into account that previous CC[0] value may be just now and it may expire between event being cleared and setting new CC[0] value. You could look at the algorithm in #15510 (https://github.com/zephyrproject-rtos/zephyr/blob/027cd2b9cb2d1091b93d9b9d1356bebda3f2b061/drivers/counter/counter_nrfx_rtc.c#L168) which also takes care of late setting (adding delta would not be needed).

In this case the problem would only arise if COUNTER incremented twice after now was read, with one of those times being after the check that COMPARE was clear. In that case we'd be at most 2 ticks late. The alternative approach you cite is extremely complicated and I strongly expect the logic required to implement it would have a worse impact than an extremely rare delay in an alarm that's already subject to latency from arbitrary interrupts.

On the other hand current approach has following consequences:

• enforcing high priority of RTC interrupt because access to RTC registers must not be interrupted (COUNTER_MIN_DELTA)
• User callbacks are also executed in RTC interrupt context. That is risky and may impact system performance. Especially, if application requires other RT operations in high priority interrupts. For unaware users that may have even more consequences (e.g. using printk from timeout callback leading to bluetooth disconnections).
• delaying expiration of late alarms by 4 cycles.

RTC interrupt should rather have low/lowest priority with algorithm that handles late setting.

The alternative approach you cite is extremely complicated

well, it's like 15-20 lines of code, if that would allow lowering interrupt priority i would go for that.

RTC registers are protected by a spinlock (disabling interrupts); the priority of the RTC interrupt is not changed by this implementation. Nor is the ability for applications to provide callbacks that are executed from within the ISR. The delay exists as an edge case and is due to limitations in the Nordic hardware; by using a dedicated peripheral rather than channel on peripheral, could be eliminated by the technique you suggested.

If something like k_alarm is to be a replacement for the existing timer infrastructure your concerns should be considered in the context of defining scope and functional requirements that can be used as the basis of target-specific implementation.

I don't see the need for having READY state. It seems to me like this comes from algorithm you've chosen which first determines list of ready timers and then processes it. If algorithm would use only one list and pick head, determine if it expired or not and call user handler there would be no READY state. Only active. READY state is used to determine during cancellation if alarm already expired but it could be determined (more precisely) by comparing alarm value vs now.

Having two queues is critical to correctly handle cases where a callback reschedules itself or another alarm, as @pizi-nordic should be able to confirm based on past arguments about the way k_timer works.

according to API doc:

if the alarm had reached its deadline but the callback had not been invoked at the point where the cancellation was requested.

but here now vs value is not checked. That would be better "lateness" indicator than state.

Doesn't have to check. If it's READY it's on the ready queue and hasn't been processed, so it can simply be removed.

the case where we see that alarm is READY happens when we interrupted RTC interrupt so we are in higher priority interrupt. We may as well interrupted RTC interrupt before ready state is determined and alarm also may be already late and that case is not covered and from user perspective they are the same.

The alarm is also observably READY when multiple alarms reach their deadline in the same timer ISR, and one callback tries to cancel another alarm while it's still on the queue.

I think this can be resolved by clarifying that "reached its deadline" does not just mean that the RTC counter has advanced to the deadline (which we can't easily observe), but that the alarm and timer infrastructure has recognized that event has happened (which we can observe, because it's reflected in the alarm state being READY).

I will reword the documentation to make that clear.

This relates to your comment about the return value from k_alarm_cancel(). As a user what I want to know is what the state of the alarm was (distinguish SCHEDULED from READY), not a difference in ticks that was captured at some arbitrary point in the past. Any such difference is irrelevant since we don't know how long ago it was calculated.

i don't see a benefit of having distinction between return value 0 and positive. We managed to cancel alarm which was in the state that would result in expire callback or we canceled alarm which would not expire because it was not active (not started or already expired). We can check if we reach alarm by reading k_cycle_get32() and comparing it against alarm->deadline. Maybe we need k_alarm_get_deadline() API?

i don't see a benefit of having distinction between return value 0 and positive.

And that's why counter cancellation doesn't provide this information and #12624/#12625 stay open. Please understand that I do find knowing the state of the alarm to be useful, and it's not helpful to make me (as an application developer) compare values (one of which changes asynchronously) and try to reconstruct it when I could just have been given the information.

I've never needed k_alarm_get_deadline(). Because this API doesn't use relative alarms it's always the value I last set it to, and I can read that from the k_alarm data structure.

i agree that knowing the state of the alarm may be important but it's just that this information is not reliable because by the time i get the information deadline may already be met (but was not when alarm was canceled). maybe k_alarm_cancel should return:

• -EINVAL if alarm is not SCHEDULED or ACTIVE
• number of ticks before deadline when alarm was canceled

That would be more or less the same but positive value would have additional information.
Note that returning 0 may only happen in higher priority interrupt. Number of ticks left would allow conditions like if ((u32_t)ret < 2) alarm_reached();

See #16398 (comment).

Can you provide a use case where having the time remaining at the point of cancellation provides more value than simply knowing the state of the alarm when it was cancelled, given that you don't know when the cancellation occurred?

I will ask the similar question 😄 What is the use case for knowing that alarm infrastructure marked alarm as ready. If cancellation returns positive value, how can you be sure that it did not expire by the time you process that information (you may be interrupted for multiple ticks). If it is vital to do something special when cancelled alarm more or less reached deadline (but was cancelled before callback) you can lock, get now compare it with deadline and if it is close (user decides how close) you do something.

Why there is alarm_update_deadline_() if it is used only here?

I think it was used elsewhere in earlier versions. I also think it's clearer as a separate function than in the body of a conditional, but I do think it should be made ALWAYS_INLINE since there's a single call site and a stupid compiler might fail to do the right thing.

remove the _ at the end of the name. (This is not used in any other kernel function afaik)

same here

same here

no need for this ifdef.

See CONFIG_POLL for instance

aerate please, no need to coalesce } and next lines (unless it's another '}')

Apply that everywhere.

Never declare a variable in the middle of a code block. (apply that everywhere)

Can you elaborate where this "never" comes from? This uninitialised variable safety issue was fixed in C99, twenty years ago.

put the && (or || etc...) at the end of the last line. See other kernel code (or everywhere else actually). Apply that everywhere